feat(aws_s3 source): Support vhost-style S3 bucket addressing #22475
Conversation
github-actions
bot
added
domain: sources
| /// | ||
| /// This controls if the bucket name is in the hostname or part of the URL. | ||
| #[serde(default = "crate::serde::default_true")] | ||
| pub force_path_style: bool, |
There was a problem hiding this comment.
Nit: It would be cleaner if this was an enum. I missed this #21999.
There was a problem hiding this comment.
Example:
vector/src/sinks/elasticsearch/config.rs
Lines 42 to 53 in 78adec7
There was a problem hiding this comment.
Yeah I was going to say, it's to be consistent with what the sink does. Do you want me to cross-implement in both the source and sink?
There was a problem hiding this comment.
I think we can accept as is (with the bool) and in the future deprecate all the bools and introduce enums. You don't have to do the later, but it would be greatly appreciated if you did :)
There was a problem hiding this comment.
I'll leave this change for the next version round, since the bool version was already approved for the sink, and the bool version for the source is approved here. I think if things are okay with the first version, I'll concurrently change the default on both the sink and source to use vhost format. It'd be a breaking change anyway to switch from bool to an enum, so changing the default from path to vhost is about the same level I'd think.
There was a problem hiding this comment.
Agree to keep this as is. Note here, that we can avoid breaking the behavior here by introducing new fields and deprecate the old ones. See https://github.com/vectordotdev/vector/blob/master/docs/DEPRECATION.md
Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>
pront
changed the title
There was a problem hiding this comment.
Good to go after https://github.com/vectordotdev/vector/actions/runs/13474840462/job/37652711395?pr=22475 is fixed. Thanks @sbalmos
|
The spell checker seems to be hung up on my username in the authors of the changelog fragment file, and the use of the word |
|
Heh, and now the |
Head branch was pushed to by a user without write access
Summary
This adds support for vhost-style S3 bucket addressing in the
aws_s3source, equivalent to the support added in theaws_s3sink in #21999.Change Type
Is this a breaking change?
How did you test this PR?
Does this PR include user facing changes?
Checklist
make check-allis a good command to run locally. This check isdefined here. Some of these
checks might not be relevant to your PR. For Rust changes, at the very least you should run:
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo nextest run --workspace(alternatively, you can runcargo test --all)Cargo.lock), pleaserun
dd-rust-license-tool writeto regenerate the license inventory and commit the changes (if any). More details here.References
#21999